Skip to content

events: add a few tests#35806

Closed
benjamingr wants to merge 1 commit intonodejs:masterfrom
benjamingr:event-target-tests
Closed

events: add a few tests#35806
benjamingr wants to merge 1 commit intonodejs:masterfrom
benjamingr:event-target-tests

Conversation

@benjamingr
Copy link
Member

@benjamingr benjamingr commented Oct 26, 2020

Added a few EventTarget tests I've found by playing with the code and "porting" bits of the spec I've read.

Note that at the moment Chrome has different prototype keys for Event though I could not figure out if that bit is actually required by the spec.

Object.keys(Object.getPrototypeOf(new Event('foo'))); // different between chrome and Node
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@benjamingr benjamingr added the events Issues and PRs related to the events subsystem / EventEmitter. label Oct 26, 2020
@benjamingr benjamingr requested a review from jasnell October 26, 2020 09:13
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 26, 2020
@targos
Copy link
Member

targos commented Oct 26, 2020 •
edited
Loading

Port a few tests from what (commit message)?

@benjamingr benjamingr changed the title events: port a few tests from events: add a few tests Oct 26, 2020
@benjamingr
Copy link
Member Author

benjamingr commented Oct 26, 2020

I initially wanted to port tests from chromium but couldn't find relevant ones - so I just went over the spec looking for bits we don't cover and added tests for them.

@codecov-io
Copy link

codecov-io commented Oct 26, 2020

Codecov Report

Merging #35806 into master will decrease coverage by 8.97%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #35806      +/-   ##
==========================================
- Coverage   96.87%   87.89%   -8.98%     
==========================================
  Files         212      477     +265     
  Lines       69486   113171   +43685     
  Branches        0    25427   +25427     
==========================================
+ Hits        67315    99475   +32160     
- Misses       2171     7994    +5823     
- Partials        0     5702    +5702
Impacted Files Coverage Δ
lib/internal/crypto/scrypt.js 28.33% <0.00%> (-71.67%) ⬇️
lib/internal/idna.js 55.55% <0.00%> (-11.12%) ⬇️
lib/internal/blocklist.js 88.70% <0.00%> (-10.49%) ⬇️
lib/internal/crypto/diffiehellman.js 89.36% <0.00%> (-9.61%) ⬇️
lib/internal/modules/esm/get_source.js 83.33% <0.00%> (-7.15%) ⬇️
lib/internal/modules/esm/get_format.js 86.11% <0.00%> (-6.95%) ⬇️
lib/internal/streams/legacy.js 93.85% <0.00%> (-6.15%) ⬇️
lib/internal/crypto/keys.js 93.32% <0.00%> (-5.89%) ⬇️
lib/internal/crypto/util.js 94.32% <0.00%> (-5.68%) ⬇️
lib/internal/histogram.js 79.78% <0.00%> (-5.32%) ⬇️
... and 405 more

jasnell
jasnell reviewed Oct 26, 2020
View reviewed changes
@benjamingr benjamingr mentioned this pull request Oct 26, 2020
Closed
3 tasks
@benjamingr benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 28, 2020
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
ExE-Boss
ExE-Boss reviewed Oct 28, 2020
View reviewed changes
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2020
@benjamingr
Copy link
Member Author

benjamingr commented Nov 3, 2020

Rebased

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2020
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 3, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 3, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/34039/

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 3, 2020
@github-actions github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 3, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2020

Commit Queue failed
- Loading data for nodejs/node/pull/35806
✔  Done loading data for nodejs/node/pull/35806
----------------------------------- PR info ------------------------------------
Title      events: add a few tests (#35806)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     benjamingr:event-target-tests -> nodejs:master
Labels     author ready, events, test
Commits    1
 - events: add a few tests
Committers 1
 - Benjamin Gruenbaum 
PR-URL: https://github.com/nodejs/node/pull/35806
Reviewed-By: Rich Trott 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35806
Reviewed-By: Rich Trott 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - events: add a few tests
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-11-03T14:36:19Z: https://ci.nodejs.org/job/node-test-pull-request/34039/
- Querying data for job/node-test-pull-request/34039/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Mon, 26 Oct 2020 09:13:53 GMT
   ✔  Approvals: 2
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35806#pullrequestreview-516666200
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/35806#pullrequestreview-516905787
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/343910509

@benjamingr
Copy link
Member Author

benjamingr commented Nov 3, 2020 •
edited
Loading

@aduh95 can you LGTM after the rebase so I can land with the commit queue? (assuming you think the changes are LGTM)

aduh95
aduh95 approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 3, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 3, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2020

Landed in 1bc1f84...ee749ba

@github-actions github-actions bot closed this Nov 3, 2020
nodejs-github-bot pushed a commit that referenced this pull request Nov 3, 2020
@benjamingr @nodejs-github-bot
events: add a few tests
ee749ba 
PR-URL: #35806
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Nov 4, 2020
@benjamingr @targos
events: add a few tests
1cdfaa8 
PR-URL: #35806
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos mentioned this pull request Nov 4, 2020
Merged
@ExE-Boss ExE-Boss mentioned this pull request Feb 5, 2021
Merged
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
@benjamingr @targos
events: add a few tests
e6443f2 
PR-URL: nodejs#35806
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos mentioned this pull request Apr 24, 2021
Closed
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
@benjamingr @targos
events: add a few tests
6eff327 
PR-URL: nodejs#35806
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
@benjamingr @targos
events: add a few tests
01d2b67 
PR-URL: nodejs#35806
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
events: add a few tests
6558ffa 
PR-URL: #35806
Backport-PR-URL: #38386
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos added backported-to-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v14.x labels Apr 30, 2021
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@aduh95 aduh95 aduh95 approved these changes

+1 more reviewer

@ExE-Boss ExE-Boss ExE-Boss left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

events Issues and PRs related to the events subsystem / EventEmitter. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@benjamingr @targos @codecov-io @nodejs-github-bot @aduh95 @jasnell @Trott @ExE-Boss